Skip to content

Don't prevent installing symfony/translation 4.2! #323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

nicolas-grekas
Copy link

What a bad idea here, it almost the same as marking the bundle as abandoned...
4.2 is going to be EOLed in 5 days.

What a bad idea here, it almost the same as marking the bundle as abandoned...
4.2 is going to be EOLed in 5 days.
@nicolas-grekas nicolas-grekas changed the title Don't prevent installing symfony/translation 4.3! Don't prevent installing symfony/translation 4.2! Jul 26, 2019
@XWB
Copy link
Contributor

XWB commented Jul 26, 2019

@nicolas-grekas As you can see in #322, this bundle doesn't work well with Symfony 4.2

@nicolas-grekas
Copy link
Author

nicolas-grekas commented Jul 26, 2019

And so the bundle works only with non-maintained versions of Symfony (4)...

@rvanlaak
Copy link
Member

#313 was merged to get tests green again since 4.2 was released.

Would it be possible to list what would be needed for making this PR mergeable?

https://symfony.com/blog/new-in-symfony-4-2-translation-related-improvements

@nicolas-grekas
Copy link
Author

#313 was merged to get tests green again since 4.2 was released.

I read that, and I think that's not a good idea. If things break with 4.2, it might be a BC break in 4.2. Blacklisting the issue is missing the chance to put pressure on Symfony to fix it.

Note that's what we do on symfony/symfony with e.g Doctrine: when our CI is green because of it, we report back, send patches, etc - anything to help make things green again by patching Doctrine.

It's a matter of policy. I think the policy that has prevailed here should be updated if this situation happens again.

@XWB
Copy link
Contributor

XWB commented Jul 26, 2019

Would it be possible to list what would be needed for making this PR mergeable?

I'm not entirely sure, but it seems that the introduction of the ICU MessageFormat in Symfony 4.2 broke the test suite. You can see the error in Travis.

@welcoMattic
Copy link
Member

It seems the domain key of MessageCatalog that breaks tests

 Symfony\Component\Translation\MessageCatalogue Object (
     'messages' => Array (
         'messages' => Array (
             'a' => 'old_a'
             'b' => 'old_b'
+            'c' => 'new_c'
+            'd' => 'old_d'
         )
-        'messages+intl-icu' => Array (...)

@welcoMattic
Copy link
Member

welcoMattic commented Jul 26, 2019

I'm looking for fixing 4.2+ compatibility, but I found something weird in a Symfony test:

https://github.com/symfony/translation/blob/master/Tests/Catalogue/MergeOperationTest.php#L56

For me, those Catalogues

new MessageCatalogue('en', ['messages' => ['a' => 'old_a', 'b' => 'old_b'], 'messages+intl-icu' => ['d' => 'old_d']]),
new MessageCatalogue('en', ['messages+intl-icu' => ['a' => 'new_a', 'c' => 'new_c']])

once merged should return

new MessageCatalogue('en', [
    'messages' => ['a' => 'old_a', 'b' => 'old_b'],   
    'messages+intl-icu' => ['d' => 'old_d', 'a' => 'new_a', 'c' => 'new_c'],
])

WDYT @nicolas-grekas?

FYI, this fix not make our tests green, but it changes the error:

1) Translation\Bundle\Tests\Unit\Catalogue\Operation\ReplaceOperationTest::testGetResultFromIntlDomain
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
         'messages' => Array (
             'a' => 'old_a'
             'b' => 'old_b'
+            'c' => 'new_c'
+            'd' => 'old_d'
         )
         'messages+intl-icu' => Array (...)
     )

/home/welcomattic/workspace/php-translation/symfony-bundle/vendor/symfony/translation/Tests/Catalogue/MergeOperationTest.php:66

Now, we have to fix those c and d rows that must not be in messages array, but only in messages+intl-icu one.

@byhaskell
Copy link

byhaskell commented Aug 7, 2019

I did not understand what the problem of working with the symphony bundle. Now, on symfony 4.3, I fixed the php-translation / symfony-bundle / Resources / views / SymfonyProfiler / translation.html.twig temlate (changed the panel to a block) and everything works fine in practice.
Block panelContent renamed to panel.

@bocharsky-bw
Copy link
Member

@byhaskell The issue with the new Intl Formatter, not with template changes, see broken tests in this PR.

@bobdenotter
Copy link
Contributor

Hi,

I too would like to see this resolved soon. In our project we use this library, and it's stuck at 0.8.1. While it might not be fully compatible with SF 4.3 yet, it does already have a number of significant fixes in 0.8.2 and dev-master, that we can't pull in right now.

@XWB
Copy link
Contributor

XWB commented Sep 5, 2019

Should be fixed by #327

@XWB XWB closed this Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants